Review requested:

• @nodejs/single-executable
• @nodejs/test_runner

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @avivkeller.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

Nice! This is a great addition. Since it's such a large PR, this will take me some time to review. Will try to tackle it over the next week.

Left an initial review, but like @Ethan-Arrowood said, it'll take time for a more in depth look

It's nice to see some momentum in this area, though from a first glance it seems the design has largely overlooked the feedback from real world use cases collected 4 years ago: https://github.com/nodejs/single-executable/blob/main/docs/virtual-file-system-requirements.md - I think it's worth checking that the API satisfies the constraints that users of this feature have provided, to not waste the work that have been done by prior contributors to gather them, or having to reinvent it later (possibly in a breaking manner) to satisfy these requirements from real world use cases.

Codecov Report

❌ Patch coverage is 92.99967% with 635 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.82%. Comparing base (e0928d6) to head (b047092).
⚠️ Report is 26 commits behind head on main.

@@            Coverage Diff             @@
##             main   #61478      +/-   ##
==========================================
+ Coverage   89.68%   89.82%   +0.13%     
==========================================
  Files         676      692      +16     
  Lines      206555   215687    +9132     
  Branches    39552    41259    +1707     
==========================================
+ Hits       185249   193739    +8490     
- Misses      13444    14061     +617     
- Partials     7862     7887      +25     

And why not something like OPFS aka whatwg/fs?

const rootHandle = await navigator.storage.getDirectory()
await rootHandle.getFileHandle('config.json', { create: true })
fs.mount('/app', rootHandle) // to make it work with fs
fs.readFileSync('/app/config.json')

OR

const rootHandle = await navigator.storage.getDirectory()
await rootHandle.getFileHandle('config.json', { create: true })

fs.readFileSync('sandbox:/config.json')

fs.createVirtual seems like something like a competing specification

And why not something like OPFS aka whatwg/fs?

I generally prefer not to interleave with WHATWG specs as much as possible for core functionality (e.g., SEA). In my experience, they tend to perform poorly on our codebase and remove a few degrees of flexibility. (I also don't find much fun in working on them, and I'm way less interested in contributing to that.)

On an implementation side, the core functionality of this feature will be identical (technically, it's missing writes that OPFS supports), as we would need to impact all our internal fs methods anyway.

If this lands, we can certainly iterate on a WHATWG-compatible API for this, but I would not add this to this PR.

Small prior art: https://github.com/juliangruber/subfs

I also worked on this a bit on the side recently: Qard@73b8fc6

That is very much in chaotic ideation stage with a bunch of LLM assistance to try some different ideas, but the broader concept I was aiming for was to have a VirtualFileSystem type which would actually implement the entire API surface of the fs module, accepting a Provider type to delegate the internals of the whole cluster of file system types to a singular class managing the entire cluster of fs-related types such that the fs module could actually just be fully converted to:

module.exports = new VirtualFileSystem(new LocalProvider())

I intended for it to be extensible for a bunch of different interesting scenarios, so there's also an S3 provider and a zip file provider there, mainly just to validate that the model can be applied to other varieties of storage systems effectively.

Keep in mind, like I said, the current state is very much just ideation in a branch I pushed up just now to share, but I think there are concepts for extensibility in there that we could consider to enable a whole ecosystem of flexible storage providers. 🙂

Personally, I would hope for something which could provide both read and write access through an abstraction with swappable backends of some variety, this way we could pass around these virtualized file systems like objects and let an ecosystem grow around accepting any generalized virtual file system for its storage backing. I think it'd be very nice for a lot of use cases like file uploads or archive management to be able to just treat them like any other readable and writable file system.

Personally, I would hope for something which could provide both read and write access through an abstraction with swappable backends of some variety, this way we could pass around these virtualized file systems like objects and let an ecosystem grow around accepting any generalized virtual file system for its storage backing. I think it'd be very nice for a lot of use cases like file uploads or archive management to be able to just treat them like any other readable and writable file system.

just a bit off topic... but this reminds me of why i created this feature request:
Blob.from() for creating virtual Blobs with custom backing storage

Would not lie, it would be cool if NodeJS also provided some type of static Blob.from function to create virtual lazy blobs. could live on fs.blobFrom for now...

example that would only work in NodeJS (based on how it works internally)

const size = 26

const blobPart = BlobFrom({
  size,
  stream (start, end) {
    // can either be sync or async (that resolves to a ReadableStream)
    // return new Response('abcdefghijklmnopqrstuvwxyz'.slice(start, end)).body
    // return new Blob(['abcdefghijklmnopqrstuvwxyz'.slice(start, end)]).stream()
    
    return fetch('https://httpbin.dev/range/' + size, {
      headers: {
        range: `bytes=${start}-${end - 1}`
      }
    }).then(r => r.body)
  }
})

blobPart.text().then(text => {
  console.log('a-z', text)
})

blobPart.slice(-3).text().then(text => {
  console.log('x-z', text)
})

const a = blobPart.slice(0, 6)
a.text().then(text => {
  console.log('a-f', text)
})

const b = a.slice(2, 4)
b.text().then(text => {
  console.log('c-d', text)
})

x-z xyz
a-z abcdefghijklmnopqrstuvwxyz
a-f abcdef
c-d cd

An actual working PoC

(I would not rely on this unless it became officially supported by nodejs core - this is a hack)

const blob = new Blob()
const symbols = Object.getOwnPropertySymbols(blob)
const blobSymbol = symbols.map(s => [s.description, s])
const symbolMap = Object.fromEntries(blobSymbol)
const {
  kHandle,
  kLength,
} = symbolMap

function BlobFrom ({ size, stream }) {
  const blob = new Blob()
  if (size === 0) return blob

  blob[kLength] = size
  blob[kHandle] = {
    span: [0, size],

    getReader () {
      const [start, end] = this.span
      if (start === end) {
        return { pull: cb => cb(0) }
      }

      let reader

      return {
        async pull (cb) {
          reader ??= (await stream(start, end)).getReader()
          const {done, value} = await reader.read()
          cb(done ^ 1, value)
        }
      }
    },

    slice (start, end) {
      const [baseStart] = this.span

      return {
        span: [baseStart + start, baseStart + end],
        getReader: this.getReader,
        slice: this.slice,
      }
    }
  }

  return blob
}

currently problematic to do: new Blob([a, b]), new File([blobPart], 'alphabet.txt', { type: 'text/plain' })

also need to handle properly clone, serialize & deserialize, if this where to be sent of to another worker - then i would transfer a MessageChannel where the worker thread asks main frame to hand back a transferable ReadableStream when it needs to read something.

but there are probably better ways to handle this internally in core with piping data directly to and from different destinations without having to touch the js runtime? - if only getReader could return the reader directly instead of needing to read from the ReadableStream using js?

Should this live behind a CLI flag initially? It might be better to work through #62328 while the API is not exposed by default.

Should this live behind a CLI flag initially?

Honestly... wouldn't hurt.

Should this live behind a CLI flag initially? It might be better to work through #62328 while the API is not exposed by default.

The biggest risk of this PR is the patches on the modules' loading side and the fs side. You cannot put them behind a flag. The rest is just a new API with bugs (like many others).

Could those patches land separately from the flaggable part? That way it’d be much easier to ensure no regressions from either portion.

(i ofc wouldn’t suggest splitting the PR until after it was otherwise philosophically landable)

@ljharb maybe. But we would be landing code that is not tested, so we’d be in a worse stance?

Note that experimental features are covered by our threat model.

Surely either the patches are meant to be noops in isolation (in which case existing tests should be sufficient - or improved - to assure no breakage) or have independent functionality (which could ship with its own tests)?

I don’t understand why. Ease of review?

Ease of review, and if there’s any risk in patching core subsystems, that PR could also be released as a patch and then that assumption would be tested, and the VFS functionality could land separately as a minor.

Certainly not required, but it might be prudent for assuring stability.

I’m moving this back to draft. I’m doing some refactoring and simplifying some paths for ease of review. I’ll add a review guide in the PR description and an architecture diagram explaining how all pieces fit together.

Have we acknowledged and documented anywhere the limitations of a Node.js-specific VFS? For example, any native addon code, any child processes (written in either Node.js or other languages and runtimes) won't see the same view of the VFS. Which is why by default I advocate for solving these types of problems at the platform layer, i.e. Linux (including "SEA", to be honest).

For example, any native addon code, any child processes, written in either Node.js or other languages and runtimes, won't see the same view of the VFS.

This is one of the reasons why I was suggesting that rather than this global mount(...) system we instead used a dependency injection approach where APIs can accept a fs-like interface which could be the real fs module or it could be a virtualized one. If we're passing around the VFS as an object it's a lot easier to both control where it is and is not used, and also to ensure it can reach these other places like native code or workers as it would need to be written to handle it explicitly.

as it would need to be written to handle it explicitly

Which is why such an hypothetical API wouldn't be used imo.

• Many packages already rely on fs and would never be updated to support arbitrary virtual systems. And any such util package would "poison" anything that depends on them, preventing them from supporting vfs as well.

• It's very debatable that having a dependency injection model would incentivize any third party tool to actually use that model - just like the Loaders API is still mostly unsupported by third-party resolvers.

By contrast the transparent vfs has been proven to work in production for the past 6+ years (through Electron first, then more generally by Yarn PnP).

• And any such util package would "poison" anything that depends on them, preventing them from supporting vfs as well.

There's not actually that many packages which actually interact with the file system. I don't think it would actually be as hard as you suggest to guide the ecosystem to adopting a cleaner pattern. To me, giving in to a bad pattern just because it's what the ecosystem currently does is lazy. We do in fact have the ability to drive adoption of new patterns and systems, as evidenced by projects adopting the built-in test framework which was added long after other options had already "won" that use case.

• It's very debatable that having a dependency injection model would incentivize any third party tool to actually use that model - just like the Loaders API is still mostly unsupported by third-party resolvers.

Adoption of ESM itself lagged significantly for a long time. That in combination with Loaders being a moving target for a very long time further lended to that lack of adoption.

Also, it's fine if not everything supports it. I don't think it's wise to be potentially weakening our security model and increasing the surface area of things which users could break unintentionally in order to extend such functionality to code which was never intended to have such capability in the first place. In my opinion these things should be explicit. Too much magic is harmful.

I recognize that not everyone agrees with me on this though, so I won't block on that point. But I still maintain that I dislike the design decision.

To me, giving in to a bad pattern just because it's what the ecosystem currently does is lazy

Which is my point: leaving it up to the ecosystem to adapt for Node.js APIs because we couldn't make a safe design would be lazy. And a bad pattern.

Using tests isn't a good analogy because not only have they marginal use in the wild compared to Jest / Vitest (due to being a recent feature, I'll give you that, although I think it supports what I was saying), it's also not a foundational feature. It's used at the app level, never in transitive packages. You simply don't have this poisonous pattern I raised earlier.

I don't think it's wise to be potentially weakening our security model

How is that weakening it? It's very vague.

Which is my point: leaving it up to the ecosystem to adapt for Node.js APIs because we couldn't make a safe design would be lazy. And a bad pattern.

But this isn't a safe design, so what we're providing here is a bad pattern. The non-lazy thing to do would be to provide something with better defined boundaries and then put in the work to advocate for the ecosystem to adopt it. Taking the route of just trying to make a thing which fits into how things work presently is the lazy approach.

How is that weakening it? It's very vague.

I've already gone over it in detail earlier in the thread. Any dependency anywhere can add custom mounts which could hijack any fs access anywhere in the entire thread. It could easily hijack things like reading certs, reading config files, etc. Malicious dependencies could easily hijack file access for nefarious purposes.

I've already gone over it in detail earlier in the thread. Any dependency anywhere can add custom mounts which could hijack any fs access anywhere in the entire thread.

I saw that, but I also saw mentioned that the Node.js threat model explicitly permits that:

Node.js trusts everything else. Examples include:

• [...]

• The code it is asked to run, including JavaScript, WASM and native code, even if said code is dynamically loaded, e.g., all dependencies installed from the npm registry. The code run inherits all the privileges of the execution user.

Just because our threat model allows it doesn't mean it does not have a measurable impact on security. It just means we don't consider it important enough or feasible to address. But it is definitely a thing which can and probably will be attacked via malicious dependencies.

I'd agree with @jasnell re. security. It's not in our threat model and it would be worse to create the false belief that there's any assurance for them to not be hijackable, because people have already been hijacking it and it's part of the hidden contract that's widely relied upon by the ecosystem that we cannot break. It's better to have a clear contract than not having contract at all and encourage people to invent their own way of hijacking it.

For reference, see #62012 - everytime we subtly change any internal module loading order such that fs methods are cached internally, we can break popular ecosystem tools like Yarn PnP and a bunch others. Having a clear contract for vfs hooks means they can move on to use that clear contract and we no longer have to worry about these when touching internals. Not having a clear contract just means people will find their own way to hijack it that creates maintenance burden for us due to Hyrum's law.

It could easily hijack things like reading certs, reading config files, etc. Malicious dependencies could easily hijack file access for nefarious purposes.

I do not think that's so easy without explicit integration. Take reading certs for example - we don't read certs through fs, this PR (at least since my lasts review) doesn't force a C++ -> JS callback to go through VFS providers (this would be visible in code, and TBH difficult to get right even if you want to do it because the certificate initialization code can happen before calling into JS is possible, and it can even go off thread), so I don't think it'd allow that unless it's explicitly implemented. We have the same thing for credentials::SafeGetenv v.s. env->env_vars()->Get() - in C++ it's fairly clear the former would not get affected by e.g. worker customizations.